Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cirque Pinnacle usability updates #17098

Closed
wants to merge 6 commits into from

Conversation

dkao
Copy link
Contributor

@dkao dkao commented May 15, 2022

Description

Some basic usability updates for the Cirque Pinnacle circle trackpad.

  • Software data ready check for when data ready pin is not available (e.g. SPLIT_POINTING_ENABLE)
  • Set ADC attenuation according to overlay, and calibrate on initialization
  • Preprocessor directives to opt out of tap and smoothing

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label May 15, 2022
dkao added 6 commits May 15, 2022 03:26
Pinnacle scans at 100sps by default, up to 300sps with certain features
disabled. POINTING_DEVICE_TASK_THROTTLE_MS is 1 by default, so Pinnacle
is polled at 1KHz when pointing device motion pin is not available
e.g. with SPLIT_POINTING_ENABLE
Check Pinnacle's software data ready bit from status register instead
of forcing data reads in this situation.
Trackpad spazzes out otherwise.
From Cirque's sample code, ADC attenuation set to:
  2X for thick plastic curved overlay
  4X for thin flat overlay

Just a basic setting for usability at the moment.
Additional features could be introduced in the future such as a Z-value
look-up table for hover rejection.
FEEDCONFIG_3 is also used for other settings such as noise avoidance and
palm rejection, which need to be disabled to raise report rate above
100sps.
Revisit if those features are added.
For those that prefer to use a key for clicks.
Baseline should be recaptured after changing ADC attenuation, since the
initial calibration may have been performed with different settings on
power up.

TODO: Import register definitions from AnyMeas_Example/Pinnacle.h
Duration for these operations undocumented (does somebody know?)
At least let them terminate rather than freeze up the keyboard when
trackpad is disconnected.

TODO: Do early return based on touch_init as well, or instead of this.
@dkao dkao force-pushed the cirque_trackpad_basic_features branch from b07a16e to 4623565 Compare May 15, 2022 10:29
@dkao
Copy link
Contributor Author

dkao commented May 15, 2022

This got labeled as core. Should pointer related PRs target develop instead of master?

@Kriechi
Copy link
Contributor

Kriechi commented May 15, 2022

@dkao great to see other people improving the pinnacle trackpads!
I already sent #17091 with already takes care of the software data ready check and a few other minor improvements and lots of code comments that we discussed on Discord with other Cirque trackpad users. How about combining our forces?

Is the smoothing actually applicable in absolute mode? Reading the docs, I think it only applies to relative mode?

@dkao
Copy link
Contributor Author

dkao commented May 15, 2022

@Kriechi Uh oh!
Yes, definitely makes sense to coordinate our efforts. The actual changes I wanted to push are inertial cursor and circular scrolling which are after these commits, not included here because I thought those would be more contentious.
I feel a pretty big difference with smoothing disabled in absolute mode. By the way, which document are you referring to? I only saw a mention of the smoothing disable bit in the “using a stylus” app note and decided to try it out, thinking I’m somehow missing a lot of information.

@Kriechi
Copy link
Contributor

Kriechi commented May 15, 2022

@dkao luckily, I referenced all docs in my PR so in the future others can discover it more easily:
https://github.com/qmk/qmk_firmware/pull/17091/files#diff-23e6ffbdd473057b633a13eb63cf8bec7e6e99578268a3713d991974af92e9a5R4
specifically: IC-DS-150408 Pinnacle Specification (though other docs also have additional details), for FeedConfig1, Bit2:

The Filter disable bit controls whether the filtering algorithm is applied to generated data. By default the hardware filters are enabled. Cirque does not recommend disabling hardware filtering.

How about we base our PRs on top of each other? The SoftwareData Ready is already covered in mine, and the Hardware Data Ready would be something new in yours - although I'm not sure if it brings any benefit? Pinnacle at most gives new data every 10ms, so interrupt-driven would just add more complexity and requires an extra (useless?) pin... happy to hear about your real-life experience if you are using it?

About calibration, I don't think we need to explicitly call this:

Pinnacle automatically executes compensation/calibration when powered on and when triggered by specific events.

Looking forward to your circular scrolling! That sounds super interesting!

@dkao
Copy link
Contributor Author

dkao commented May 15, 2022

@Kriechi So there's a filtering disable bit in FeedConfig1 and a separate smoothing disable bit in FeedConfig3, bit 1. Filtering disable likely refers to filters on the capacitive image itself before coordinates are computed, and smoothing disable refers to a smoothing filter applied to the computed coordinates.

The SoftwareData Ready is already covered in mine, and the Hardware Data Ready would be something new in yours - although I'm not sure if it brings any benefit? Pinnacle at most gives new data every 10ms, so interrupt-driven would just add more complexity and requires an extra (useless?) pin

Hardware data ready pin is already handled in QMK by

#ifdef POINTING_DEVICE_MOTION_PIN
# if defined(SPLIT_POINTING_ENABLE)
# error POINTING_DEVICE_MOTION_PIN not supported when sharing the pointing device report between sides.
# endif
if (!readPin(POINTING_DEVICE_MOTION_PIN))
#endif
There isn't really a need to check hardware data ready pin and the data ready bits in status register.
I think it's better for the caller to check for data readiness as needed, rather than have cirque_pinnacle_read_data() send fake reports potentially throwing off caller logic (such as the tap code checking touchData.touchDown).

About calibration, I don't think we need to explicitly call this:

Pinnacle automatically executes compensation/calibration when powered on and when triggered by specific events.

We do if changing ADC attenuation, the controller doesn't automatically recalibrate.
If you're lucky, controller figures out it has a bad baseline after a few touches and recovers by triggering a recalibration on its own. If you're not lucky the trackpad remains unusable.

Let's work on your PR first, I'll rebase my stuff on top of that.

@dkao dkao closed this May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants